r? @jackh726

(rust-highfive has picked a reviewer for you, use r? to override)

r? @nnethercote

Some comparisons of rustc-perf results from local x86_64 linux machine with and without LLVM inlining controlled with the newly added inline-llvm option when inline-mir=yes is set:

Instructions:u of all benchmarks with Profile : opt, Scenario : full, Category : Primary

Those results don't seem unexpected to me, but the real question I guess is the performance impact of disabling LLVM inlining -- one way to try to gauge that is to adjust our CI to run without it, and then see how much slower the produced rustc binaries are. My guess is "much slower, >50%", but I don't really know.

For some context: I suggested this as a good first task for @yanchen4791 to look into, based on an idea from @joshtriplett. In the next day or two I will look at both the compile time and runtime effects.

Thanks for filing the PR, @yanchen4791!

Thanks for advice, @nnethercote !

Tested the rustc binary produced with LLVM inlining disabled (and MIR inlining enabled) using the following build command:

RUSTFLAGS_NOT_BOOTSTRAP="-Z inline-mir=yes -Z inline-llvm=no" ./x.py build

The produced rustc seems running faster than the rustc produced by default (-Z inline-mir=no -Z inline-llvm=yes").

Here are comparisons of 2 versions of rustc's, using raw data of instructions:u of Primary and Secondary benchmarks. With Primary benchmarks, the total combined instructions:u reduced 5.19%, while with Secondary benchmarks, total combined instructions:u increased 0.05%. (Note: I am not using the average percentage number here, it is kind of misleading. Instead, I am using the change in percentage of total number of instructions combined all benchmarks, this I think should be a more accurate measure for rustc performance)

@yanchen4791: Can you update this PR so that inline_llvm is false by default? Then we can do a performance run on CI.

@nnethercote Sure. Make inline-llvm false by default. How about default of inline-mir ? Currently default value of inline-mir is false. Should it be changed to true for CI?

inline-mir defaults to false? I'm surprised by that. @cjgillot, can you explain? I thought #91743 enabled it...

Some inline/codegen tests failed after setting inline_llvm default to false. Here is the list:

failures:

    [ui] src/test/ui/lto/thin-lto-inlines.rs
    [ui] src/test/ui/lto/thin-lto-inlines2.rs

    [codegen] src/test/codegen/alloc-optimisation.rs
    [codegen] src/test/codegen/array-equality.rs
    [codegen] src/test/codegen/atomic-operations.rs
    [codegen] src/test/codegen/autovectorize-f32x4.rs
    [codegen] src/test/codegen/binary-search-index-no-bound-check.rs
    [codegen] src/test/codegen/catch-unwind.rs
    [codegen] src/test/codegen/dealloc-no-unwind.rs
    [codegen] src/test/codegen/inline-always-works-always.rs#NO-OPT
    [codegen] src/test/codegen/inline-hint.rs
    [codegen] src/test/codegen/issue-13018.rs
    [codegen] src/test/codegen/issue-34634.rs
    [codegen] src/test/codegen/issue-34947-pow-i32.rs
    [codegen] src/test/codegen/issue-37945.rs
    [codegen] src/test/codegen/issue-45222.rs
    [codegen] src/test/codegen/issue-45466.rs
    [codegen] src/test/codegen/issue-45964-bounds-check-slice-pos.rs
    [codegen] src/test/codegen/issue-69101-bounds-check.rs
    [codegen] src/test/codegen/issue-73396-bounds-check-after-position.rs
    [codegen] src/test/codegen/issue-75659.rs
    [codegen] src/test/codegen/issue-96497-slice-size-nowrap.rs
    [codegen] src/test/codegen/non-terminate/infinite-loop-1.rs
    [codegen] src/test/codegen/non-terminate/infinite-loop-2.rs
    [codegen] src/test/codegen/non-terminate/nonempty-infinite-loop.rs
    [codegen] src/test/codegen/repeat-trusted-len.rs
    [codegen] src/test/codegen/simd-wide-sum.rs
    [codegen] src/test/codegen/slice-as_chunks.rs
    [codegen] src/test/codegen/slice-position-bounds-check.rs
    [codegen] src/test/codegen/slice-ref-equality.rs
    [codegen] src/test/codegen/slice-reverse.rs
    [codegen] src/test/codegen/slice-windows-no-bounds-check.rs
    [codegen] src/test/codegen/some-global-nonnull.rs
    [codegen] src/test/codegen/swap-large-types.rs
    [codegen] src/test/codegen/swap-simd-types.rs
    [codegen] src/test/codegen/swap-small-types.rs
    [codegen] src/test/codegen/target-feature-overrides.rs#COMPAT
    [codegen] src/test/codegen/thread-local.rs
    [codegen] src/test/codegen/to_vec.rs
    [codegen] src/test/codegen/vec-calloc-llvm14.rs
    [codegen] src/test/codegen/vec-in-place.rs
    [codegen] src/test/codegen/vec-iter-collect-len.rs
    [codegen] src/test/codegen/vec-optimizes-away.rs
    [codegen] src/test/codegen/vec-shrink-panik.rs

@nnethercote the documentation is wrong. inline-mir defaults to None, which means "act depending on opt-level". inline-mir=yes/no will force/deny MIR inlining independently of the value of opt-level.
The flag documentation should really say "forcibly enable or disable MIR inlining."

With inline_llvm default being set to false, in addition to failures of some test cases of codegen and inline, x.py test hangs at the following stage:

     Running tests/lib.rs (build/x86_64-unknown-linux-gnu/stage1-std/x86_64-unknown-linux-gnu/release/deps/coretests-d5e01cd647bf6a6e)

running 1458 tests
........................................................................................ 88/1458
........................................................................................ 176/1458
........................................................................................ 264/1458
........................................................................................ 352/1458
........................................................................................ 440/1458
........................................................................................ 528/1458
.........................................................ii............................. 616/1458
........................................................................................ 704/1458
........................................................................................ 792/1458
........................................................................................ 880/1458
........................................................................................ 968/1458
........................................................................................ 1056/1458
........................................................................................ 1144/1458
........................................................................................ 1232/1458
........................................................................................ 1320/1458
........................................................................................ 1408/1458
............................................test slice::take_in_bounds_max_range_from has been running for over 60 seconds
test slice::take_in_bounds_max_range_to has been running for over 60 seconds
test slice::take_mut_in_bounds_max_range_from has been running for over 60 seconds
test slice::take_mut_in_bounds_max_range_to has been running for over 60 seconds
test slice::take_mut_oob_max_range_to_inclusive has been running for over 60 seconds
test slice::take_oob_max_range_to_inclusive has been running for over 60 seconds

Since we're quite interested in perf, I don't think that we have to resolve these failures outright. As long as the compiler builds, we can run perf, it doesn't require passing tests. So if you push a version that defaults to inline-llvm=false, I can start a perf. run. Of course if the perf. run fails then it will have to be fixed.

@Kobzol Sure, compiler builds without any problem. Pushed with inline_llvm defaults to false.

@bors try @rust-timer queue

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

⌛ Trying commit 7b27cbb1c4403562563661625d7ca2c38f9a4d6a with merge 9e67400a96e1cc2dc2652554e389f59791dc41b3...

☀️ Try build successful - checks-actions
Build commit: 9e67400a96e1cc2dc2652554e389f59791dc41b3 (9e67400a96e1cc2dc2652554e389f59791dc41b3)

Queued 9e67400a96e1cc2dc2652554e389f59791dc41b3 with parent aeb5067, future comparison URL.

Finished benchmarking commit (9e67400a96e1cc2dc2652554e389f59791dc41b3): comparison url.

Instruction count

• Primary benchmarks: mixed results
• Secondary benchmarks: mixed results

Max RSS (memory usage)

Cycles

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

Bootstrap timings reduced by ~30 % when they're not LLVM inlined (but the compiling rustc is normally optimized), quite cool.

I'm having trouble keeping track of which results are considered valid and what is being measured. I would be grateful if someone made a list summarizing the experiments with links to the result summaries and what they mean.

Which stages of the build do bootstrap timings include? I looked for docs on this but couldn't find any.

Which stages of the build do bootstrap timings include? I looked for docs on this but couldn't find any.

If I understand it correctly, the bootstrap timings are gathered from compiling rustc/compiler with rustc from the PR. In other words, the rustc from the PR is used as the bootstrap compiler and builds stage 1. @Mark-Simulacrum Is this correct?

I tried to gather a summary of the results:

LLVM inlining disabled for rustc and for benchmarks and bootstrap

https://perf.rust-lang.org/compare.html?start=aeb5067967ef58e4a324b19dd0dba2f385d5959f&end=9e67400a96e1cc2dc2652554e389f59791dc41b3&stat=instructions:u

LLVM inlining disabled for rustc and for benchmarks and bootstrap, MIR inlining enabled for everything

https://perf.rust-lang.org/compare.html?start=9b4ea391a132ec5f5de40079597ab7ff2fd691ad&end=b102bd401c3f4366afba3adb1b706cec1018f7c3&stat=instructions:u

LLVM inlining enabled for rustc, disabled for benchmarks and bootstrap, MIR inlining enabled for everything

https://perf.rust-lang.org/compare.html?start=4d45b0745ab227feb9000bc15713ade4b99241ea&end=fc06468a9d6916240c025225bacc5f19817e4781&stat=instructions:u

LLVM inlining enabled for rustc, disabled for benchmarks and bootstrap, MIR inlining enabled for opt builds

https://perf.rust-lang.org/compare.html?start=7480389611f9d04bd34adf41a2b3029be4eb815e&end=b0cf6e1fb74239b3778b894185e4d7af516653ad&stat=instructions:u

LLVM inlining enabled for rustc and std, disabled for benchmarks and boostrap, MIR inlining enabled for everything

https://perf.rust-lang.org/compare.html?start=76f3b891a0c48e128c5a43ef57e70d86735e1cf2&end=7714bc32bda08036b7d1f2d88bedaa53bb269901&stat=instructions:u

I couldn't easily copy the summary tables, so I only posted links here. I'm not sure whether the inlining for std affects anything.

@nnethercote I've cleaned up testing code and rebased the branch. Also changed the documentation of inline-mir default from "no" to "off" since the default value of inline_mir is None, not Some(false).

It just needs the off occurrences to be changed back to no and then it should be good. Thanks.

@bors r+ rollup=always

📌 Commit 052887e has been approved by nnethercote

It is now in the queue for this repository.

⌛ Testing commit 052887e with merge c0ec2e0a4576203fb7a9cd9377cb293617939fb7...

💔 Test failed - checks-actions

@bors retry

A job failed! Check out the build log: (web) (plain)